Skip to content

Conversation

@AlexanderGrissik
Copy link
Collaborator

Description

Removing IPoIB data and control pathes.

What

IPoIB is not supported any more due to restrictions in the mlx5 driver.

Why ?

Obsolete features removal

Change type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Tests
  • Other

Check list

  • Code follows the style de facto guidelines of this project
  • Comments have been inserted in hard to understand places
  • Documentation has been updated (if necessary)
  • Test has been added (if possible)

@AlexanderGrissik
Copy link
Collaborator Author

Pay attention to changes for:

  1. RX_BUF_SIZE(mtu)
  2. neigh_observer

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Overview

Greptile Summary

This PR removes IPoIB (IP over InfiniBand) support from libvma, as it's no longer supported by the mlx5 driver. The changes are comprehensive and touch 55 files, removing ~2,821 lines of code.

Key Changes

  • Removed entire IPoIB data and control paths: Deleted neigh_ib, qp_mgr_ib, net_device_val_ib, IPoIB_addr classes
  • Deleted IGMP handling: Removed igmp_handler.cpp and igmp_mgr.cpp (only needed for IPoIB multicast)
  • Simplified packet processing: Removed all transport type checks - code now assumes Ethernet only
  • Updated device validation: Replaced verify_ipoib_or_eth_qp_creation() with verify_eth_qp_creation()
  • Removed IPoIB-specific macros: Cleaned up IPOIB_* constants, ipoibhdr struct, GRH_HDR_LEN, etc.
  • Simplified flow steering: Multicast flow rules now only support Ethernet (removed IB UD flow specs)
  • Build configuration: Removed IBV_FLOW_SPEC_IB capability checks and IPoIB source files from Makefile

Impact

This is a breaking change that removes support for InfiniBand datagram mode interfaces. Applications using VMA on IPoIB interfaces will no longer work. The change is well-documented in the PR description as an "obsolete features removal" due to mlx5 driver restrictions.

Confidence Score: 5/5

  • This PR is safe to merge - it's a clean, systematic removal of obsolete IPoIB support
  • The removal is thorough and well-executed across all layers. All IPoIB-specific code paths have been cleanly eliminated including classes, functions, macros, and build configurations. The code now assumes Ethernet-only transport, which simplifies the codebase. No logic errors or incomplete removals detected.
  • No files require special attention - the removal is consistent and complete across all affected files

Important Files Changed

File Analysis

Filename Score Overview
src/vma/util/vtypes.h 5/5 Removed IPoIB-related macros and constants (IPOIB_HW_ADDR_*, ipoibhdr struct, GRH_HDR_LEN, etc.). Clean removal of unused definitions.
src/vma/proto/neighbour.cpp 5/5 Removed neigh_ib class and neigh_ib_val operator. Changed buffer size from IPOIB_HW_ADDR_LEN to ETH_ALEN. Only Ethernet neighbor handling remains.
src/vma/proto/L2_address.h 5/5 Removed IPoIB_addr class entirely. Only ETH_addr class remains for L2 address handling.
src/vma/dev/net_device_val.cpp 5/5 Removed net_device_val_ib class and all IPoIB device handling. Removed verify_enable_ipoib(), removed ARPHRD_INFINIBAND handling. Only Ethernet device support remains.
src/vma/dev/qp_mgr.cpp 5/5 Removed qp_mgr_ib class and all IB UD QP handling. Removed fictive AH creation in trigger_completion. Only RAW_PACKET QP support remains.
src/vma/dev/cq_mgr.cpp 5/5 Removed transport_type field and GRH_HDR_LEN handling. Removed is_ib_tcp_frame() checks. Hardcoded to ETH_HDR_LEN for prefetch calculations.
src/vma/dev/ring_slave.cpp 5/5 Removed IPoIB transport type handling in rx_process_buffer(). Removed IGMP protocol handling. Removed transport_type switch statement - now assumes Ethernet only.
src/vma/dev/rfs_mc.cpp 5/5 Removed IB multicast flow steering. Changed prepare_flow_spec() from bool to void. Only Ethernet multicast flow rules remain.

Sequence Diagram

sequenceDiagram
    participant App as Application
    participant VMA as VMA Library
    participant NetDev as net_device_val
    participant QP as qp_mgr
    participant CQ as cq_mgr
    participant Ring as ring_slave
    participant RFS as rfs_mc/rfs_uc

    Note over VMA,RFS: IPoIB Support Removal

    App->>VMA: Initialize VMA
    VMA->>NetDev: configure()
    NetDev->>NetDev: verify_eth_qp_creation()
    Note right of NetDev: Previously: verify_ipoib_or_eth_qp_creation()<br/>Now: Only Ethernet validation

    NetDev->>NetDev: create_L2_address()
    Note right of NetDev: Only ETH_addr created<br/>IPoIB_addr class removed

    NetDev->>QP: configure()
    QP->>QP: prepare_ibv_qp()
    Note right of QP: Only IBV_QPT_RAW_PACKET<br/>IBV_QPT_UD removed

    QP->>CQ: init_rx_cq_mgr()
    CQ->>CQ: configure()
    Note right of CQ: Hardcoded ETH_HDR_LEN<br/>GRH_HDR_LEN handling removed

    App->>VMA: Multicast Join
    VMA->>RFS: attach_flow()
    RFS->>RFS: prepare_flow_spec()
    Note right of RFS: Only Ethernet MC flow rules<br/>IB MC flow specs removed

    App->>VMA: Send/Receive Data
    VMA->>Ring: rx_process_buffer()
    Ring->>Ring: Validate Ethernet header
    Note right of Ring: No IPoIB header validation<br/>No IGMP processing
    Ring->>Ring: Process IP packet
    Ring->>App: Deliver data

    Note over VMA,RFS: All IPoIB code paths eliminated
Loading

55 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link
Collaborator

@tomerdbz tomerdbz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work Grissik. some small questions and 2 opportunities for optimizations

@greptile-apps
Copy link

greptile-apps bot commented Nov 11, 2025

Greptile Overview

Greptile Summary

Removed all IPoIB (IP over InfiniBand) support from libvma, eliminating both data and control paths for IPoIB interfaces due to mlx5 driver restrictions.

Major Changes:

  • Removed IPoIB-specific classes: neigh_ib, neigh_ib_broadcast, neigh_ib_val, IPoIB_addr, net_device_val_ib, qp_mgr_ib, ring_ib, ring_bond_ib
  • Deleted IGMP manager and handler (218 + 124 lines) used for IPoIB multicast management
  • Removed IB-specific WQE handler (wqe_send_ib_handler) for InfiniBand work queue element processing
  • Eliminated transport type abstraction layer - simplified from dual ETH/IB support to ETH-only
  • Removed configuration variable enable_ipoib and associated environment variable handling
  • Simplified neighbor resolution to always use Ethernet-style addressing (removed AH/qkey/qpn handling)
  • Removed IBV_QPT_UD (Unreliable Datagram) QP support, retained only IBV_QPT_RAW_PACKET for Ethernet
  • Cleaned up dst_entry to remove transport-dependent path selection logic
  • Updated build system to exclude removed source files

Code Quality:
The removal is thorough and clean. All IPoIB code paths have been properly eliminated, with no dangling references to removed classes or functions (verified by checking for neigh_ib, ring_ib, net_device_val_ib references). The remaining code correctly assumes Ethernet-only operation.

Confidence Score: 5/5

  • This PR is safe to merge - it's a clean removal of obsolete IPoIB support with no functional issues
  • The removal is comprehensive and well-executed. All IPoIB-specific code has been systematically eliminated including classes, methods, configuration variables, and build targets. The code now assumes Ethernet-only operation throughout, which simplifies the architecture. No incomplete removals or dangling references were found. The changes align with the stated goal of removing obsolete features due to mlx5 driver restrictions.
  • No files require special attention - all changes are straightforward deletions

Important Files Changed

File Analysis

Filename Score Overview
src/vma/proto/neighbour.cpp 5/5 Removed 628 lines of IB neighbor handling (neigh_ib, neigh_ib_broadcast classes), retained only neigh_eth implementation
src/vma/proto/neighbour.h 5/5 Removed neigh_ib_val, neigh_eth_val classes and transport_type field, simplified to single neigh_val implementation
src/vma/proto/dst_entry.cpp 5/5 Removed transport type selection logic, conf_l2_hdr_and_snd_wqe_ib(), simplified to always use ETH path
src/vma/dev/qp_mgr.cpp 5/5 Removed 141 lines of IB QP management (qp_mgr_ib class), pkey handling, and UD QP creation logic
src/vma/dev/net_device_val.cpp 5/5 Removed 183 lines including net_device_val_ib class, ARPHRD_INFINIBAND handling, and IPoIB validation logic
src/vma/proto/igmp_handler.cpp 5/5 File removed entirely (218 lines) - IGMP handling for IPoIB multicast no longer needed
src/vma/proto/igmp_mgr.cpp 5/5 File removed entirely (124 lines) - IGMP manager for IPoIB multicast no longer needed
src/vma/Makefile.am 5/5 Removed build targets for wqe_send_ib_handler, igmp_mgr, igmp_handler, and neighbour_observer files

Sequence Diagram

sequenceDiagram
    participant App as Application
    participant NetDev as net_device_val
    participant Neigh as neighbour (neigh_eth)
    participant DST as dst_entry
    participant QP as qp_mgr_eth
    participant Ring as ring_simple

    Note over NetDev,Ring: IPoIB Support Removal

    App->>NetDev: initialize network device
    NetDev->>NetDev: verify_eth_qp_creation()
    Note right of NetDev: Removed: verify_ipoib_or_eth_qp_creation()<br/>Now only ETH supported
    NetDev->>NetDev: create_L2_address(ETH_addr)
    Note right of NetDev: Removed: IPoIB_addr creation

    App->>DST: prepare_to_send()
    DST->>DST: resolve_neigh()
    DST->>DST: alloc_neigh_val()
    Note right of DST: Removed: transport type selection<br/>Always allocates neigh_val (ETH)
    DST->>Neigh: register with neigh_eth
    Note right of Neigh: Removed: neigh_ib, neigh_ib_broadcast

    DST->>DST: conf_hdrs_and_snd_wqe()
    DST->>DST: conf_l2_hdr_and_snd_wqe_eth()
    Note right of DST: Removed: conf_l2_hdr_and_snd_wqe_ib()<br/>Removed: transport type switch

    App->>QP: create_qp()
    QP->>QP: prepare_ibv_qp(IBV_QPT_RAW_PACKET)
    Note right of QP: Removed: IBV_QPT_UD support<br/>Removed: pkey handling<br/>Removed: IB-specific QP modifications

    App->>Ring: send_buffer()
    Ring->>QP: post_send()
    Note right of Ring: Removed: IB AH (Address Handle)<br/>Removed: qkey/qpn handling<br/>Only ETH frames supported

    Note over App,Ring: IGMP Manager Removed<br/>WQE IB Handler Removed<br/>neighbour_observer interface simplified
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

55 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

55 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

55 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@AlexanderGrissik
Copy link
Collaborator Author

bot:retest

1 similar comment
@AlexanderGrissik
Copy link
Collaborator Author

bot:retest

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

56 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Removing IPoIB data and control pathes. IPoIB is not supported any more due to restrictions in the mlx5 driver.

Signed-off-by: Alexander Grissik <[email protected]>
Valgrind is single threaded app and does not handle well spin locks.
Adding fair-sched=yes allows Sockperf main thread and VMA internal thread to operate without being blocked by Valgrind.

Signed-off-by: Alexander Grissik <[email protected]>
@galnoam galnoam merged commit 86c40fc into Mellanox:master Nov 24, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants